fix(util): replace string parsing with AST in inspect_without_import #3504 by guptapratykshh · Pull Request #3514 · chaoss/augur

guptapratykshh · 2026-01-06T20:57:53Z

Description

Replaced the string parsing logic in augur/util/inspect_without_import.py with the ast module to robustly extract phase function names from start_tasks.py.
Added a new test suite in tests/test_util/test_inspect_without_import.py to verify the fix and ensure it handles various edge cases (indentation, decorators, multi-line definitions).
Updated comments and docstrings to be more human-readable and concise.

This PR fixes #3504

Notes for Reviewers

Signed commits

Yes, I signed my commits.

shlokgilda · 2026-01-07T08:20:56Z

Great refactor. I've added some inline comments. Other than that, could you add a note about whether you used any AI tools (Claude, Copilot, etc.) for this PR? If so, what were they used for? You can see examples from other closed PRs. But otherwise looks like good PR.

guptapratykshh · 2026-01-07T08:56:34Z

Thanks for the review. No, I did not use any AI tools for the implementation in this PR.

MoralCode

Overall looks good! just a couple pieces of feedback

Thanks for doing this!

augur/util/inspect_without_import.py

tests/test_util/test_inspect_without_import.py

augur/util/inspect_without_import.py

MoralCode · 2026-01-10T20:25:47Z

id like to test this (run the unit tests) and add them to the CI, but overall looks good!

MoralCode · 2026-01-10T20:27:23Z

augur/util/inspect_without_import.py

+            # We want any function that identifies itself as a 'phase'
+            if '_phase' in node.name:
+                phase_names.append(node.name)


does this filter for functions? or could someone create a top level variable matching this pattern and cause it to break too?

No, this will not match top level variables. The check if isinstance(node, ast.FunctionDef): filters for function definitions only. Variables show up as ast.Assign or ast.AnnAssign nodes in AST tree, so this logic completely skips them.

I just added specific unit test, test_ignores_variables_with_matching_name, to verify this behavior and ensure safety.

guptapratykshh · 2026-01-16T05:30:34Z

the changes have done , pleae have a look

MoralCode · 2026-02-04T17:23:40Z

These new unit tests are not added to pyproject.toml such that they will be automatically tested. once thats done i will do a final review pass and merge this

MoralCode · 2026-02-09T22:37:45Z

Hello! Just wanted to check in to see if you were still interested in helping the maintainers merge this PR. We noticed it has been a little while since this last had activity, and are considering closing it or taking it over if it remains in its current state.

Please react to or reply to this to confirm your interest in the next 7 days or let us know if you are no longer interested in this so we can best prioritize everyone's contributions.

Thanks!

…haoss#3504 Signed-off-by: Pratyksh Gupta <pratykshgupta9999@gmail.com> Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>

Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>

Signed-off-by: Adrian Edwards <adredwar@redhat.com>

…rsion of the code Signed-off-by: Adrian Edwards <adredwar@redhat.com>

MoralCode · 2026-02-16T18:44:49Z

Taking this over.

rebased
applied my feedback from earlier (added to the unit test file)
updated the logic to validate the output of this new function is an identical match for the current output.

MoralCode · 2026-02-16T18:46:24Z

i have tested this insofar as i was running the unit tests both before and after this change to validate both versions of the function are passing.

Happy to split this PR to merge the unit test first if that would help de-risk this or demonstrate these assertions publicly

shlokgilda

The core change is solid. The main thing blocking this for me is that most of the tests reimplement the AST logic inline with a different matching rule (in vs endswith), so they don't actually test the function. A small refactor to make the function accept an optional path would let all the edge case tests exercise the real code path.

shlokgilda · 2026-02-16T20:48:22Z

tests/test_util/test_inspect_without_import.py

+
+def    weirdly_spaced_phase   (repo_git):
+    pass
+'''


These inline tests use '_phase' in node.name (contains check), but the actual implementation uses node.name.endswith('_phase'). These are different matching rules. For eg. build_primary_phase_request contains _phase but doesn't end with it.

This matters because test_ignores_non_phase_functions explicitly asserts build_primary_phase_request IS found, which is correct under in but wrong under endswith. So the tests pass, but they're testing different logic than what ships.

Maybe we refactor get_phase_names_without_import() to accept an optional source_path parameter (defaulting to the current hardcoded path). Then these tests can call the real function with a temp file instead of reimplementing the AST walk inline. That way any change to the matching logic is automatically covered.

ah so this is probably because i changed it

the endswith logic is a closer match to the behavior existing non AST version of the function i think. i probably didnt go through all the test cases in very much detail

shlokgilda · 2026-02-16T20:52:28Z

tests/test_util/test_inspect_without_import.py

+    def test_handles_async_functions(self):
+        """Checks if async functions are also detected if they have the right name."""
+        test_code = '''
+async def async_phase(repo_git):
+    pass
+
+def sync_phase(repo_git):
+    pass
+'''
+
+        tree = ast.parse(test_code)
+        phase_names = []
+
+        # AsyncFunctionDef is a different node type than FunctionDef
+        for node in ast.walk(tree):
+            if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and '_phase' in node.name:
+                phase_names.append(node.name)
+
+        assert 'async_phase' in phase_names
+        assert 'sync_phase' in phase_names
+        assert len(phase_names) == 2


This test checks isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) but the implementation only checks ast.FunctionDef. Since this test does its own inline AST walk rather than calling the real function, it passes. But it's testing behavior the function doesn't actually have. If an async phase function were added to start_tasks.py, this test would still pass while the function silently misses it.

shlokgilda · 2026-02-16T20:54:03Z

augur/util/inspect_without_import.py

+    for node in ast.walk(tree):
+        if isinstance(node, ast.FunctionDef):
+            # We want any function that identifies itself as a 'phase'
+            if node.name.endswith('_phase'):


Most of the edge case tests use '_phase' in node.name (contains) instead of endswith('_phase'). That means the tests are validating a broader matching rule than what this actually ships. More on that in the test file comments.

Also, this only checks ast.FunctionDef. If someone ever adds an async def phase function, it'd be silently skipped. Probably fine for now since there aren't any, but worth a comment or including ast.AsyncFunctionDef in the check.

MoralCode · 2026-02-17T16:54:55Z

yeah i agree with shlok, the tests need to not re-implement things. they should exercise the actual code paths being used.

original contributor is no longer active to provide details on whether these tests were written with AI or not so we can only guess.

Might make sense to have one of us, as maintainers reimplement them

That said, this is more a tech debt reduction and generally quite low priority fix, so maybe not worth doing that for now.

shlokgilda · 2026-02-17T22:23:06Z

Hey, I've got the fixes ready on my fork but don't have push access to this branch. @MoralCode what's the best way to get these changes onto this branch? Should I open a separate PR instead?

guptapratykshh requested a review from sgoggins as a code owner January 6, 2026 20:57

guptapratykshh force-pushed the fix/issue-3504-ast-parsing branch from d466300 to 2e0e0e5 Compare January 6, 2026 20:59

MoralCode requested changes Jan 9, 2026

View reviewed changes

augur/util/inspect_without_import.py Show resolved Hide resolved

tests/test_util/test_inspect_without_import.py Show resolved Hide resolved

augur/util/inspect_without_import.py Outdated Show resolved Hide resolved

guptapratykshh force-pushed the fix/issue-3504-ast-parsing branch from 2e0e0e5 to b88985a Compare January 9, 2026 17:54

guptapratykshh requested a review from MoralCode January 10, 2026 13:06

MoralCode added the waiting This change is waiting for some other changes to land first label Jan 10, 2026

MoralCode reviewed Jan 10, 2026

View reviewed changes

guptapratykshh force-pushed the fix/issue-3504-ast-parsing branch from d865924 to 623c6ce Compare January 10, 2026 20:50

MoralCode added pending changes PRs that have review comments that have been added but havent been addressed in a while and removed waiting This change is waiting for some other changes to land first labels Jan 11, 2026

guptapratykshh force-pushed the fix/issue-3504-ast-parsing branch from b6ac1de to 8fdacaf Compare January 12, 2026 04:28

guptapratykshh requested a review from MoralCode January 14, 2026 12:26

MoralCode added the stale Stuff that's abandoned or not making forward progress and may need taking over/reassignment/closing label Feb 9, 2026

guptapratykshh and others added 4 commits February 16, 2026 13:31

fix(util): replace string parsing with AST in inspect_without_import c…

171b395

…haoss#3504 Signed-off-by: Pratyksh Gupta <pratykshgupta9999@gmail.com> Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>

test: add verification that variables matching phase pattern are ignored

c8abf99

Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>

add tests to pyproject.toml for continuous validation

bb1ef05

Signed-off-by: Adrian Edwards <adredwar@redhat.com>

update the test to validate the output is identical to the current ve…

4ec5c4e

…rsion of the code Signed-off-by: Adrian Edwards <adredwar@redhat.com>

MoralCode force-pushed the fix/issue-3504-ast-parsing branch from b374824 to 4ec5c4e Compare February 16, 2026 18:43

MoralCode self-assigned this Feb 16, 2026

MoralCode added the discussion Seeking active feedback, usually for items under active development label Feb 16, 2026

shlokgilda requested changes Feb 16, 2026

View reviewed changes

MoralCode added pending changes PRs that have review comments that have been added but havent been addressed in a while and removed ready Items tested and seeking additional approvals or a merge. Usually for items under active development labels Feb 17, 2026

Conversation

guptapratykshh commented Jan 6, 2026

Uh oh!

shlokgilda commented Jan 7, 2026

Uh oh!

guptapratykshh commented Jan 7, 2026

Uh oh!

MoralCode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MoralCode commented Jan 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guptapratykshh commented Jan 16, 2026

Uh oh!

MoralCode commented Feb 4, 2026

Uh oh!

MoralCode commented Feb 9, 2026

Uh oh!

MoralCode commented Feb 16, 2026

Uh oh!

MoralCode commented Feb 16, 2026

Uh oh!

shlokgilda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoralCode commented Feb 17, 2026

Uh oh!

shlokgilda commented Feb 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants